Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: more clippy lints #598

Merged
merged 2 commits into from
Jul 16, 2024

Conversation

aniketpr01
Copy link
Contributor

@aniketpr01 aniketpr01 commented Jun 6, 2024

Closes #596

Add linters as referenced from reth repo with below exceptions which can be added later.
Exceptions till now:
rust.missing_docs = "allow"
rust.unreachable_pub = "allow"
type-repetition-in-bound = "allow"

Added PR to ignore linter changes affecting codegen.rs
PR: xJonathanLEI/starknet-jsonrpc-codegen#5

starknet-accounts/src/factory/mod.rs Outdated Show resolved Hide resolved
starknet-accounts/src/factory/mod.rs Outdated Show resolved Hide resolved
starknet-accounts/tests/single_owner_account.rs Outdated Show resolved Hide resolved
@aniketpr01 aniketpr01 requested a review from tcoratger June 12, 2024 14:26
@tcoratger
Copy link
Collaborator

Looks good to me, just two remarks:

  • For the codegen file, this should be handle directly at source, can you confirm @xJonathanLEI ?
  • Can you rebase on main?

@tcoratger
Copy link
Collaborator

lgtm @xJonathanLEI what about codegen file?

@aniketpr01 aniketpr01 marked this pull request as ready for review June 18, 2024 08:57
@aniketpr01 aniketpr01 force-pushed the feat/add-linters branch 2 times, most recently from 2e1d496 to d9b7923 Compare June 18, 2024 09:24
@aniketpr01 aniketpr01 marked this pull request as draft June 18, 2024 09:25
@aniketpr01 aniketpr01 marked this pull request as ready for review June 18, 2024 09:25
starknet-curve/src/ec_point.rs Outdated Show resolved Hide resolved
starknet-ff/Cargo.toml Outdated Show resolved Hide resolved
@aniketpr01 aniketpr01 requested a review from tcoratger June 18, 2024 14:45
Cargo.toml Outdated Show resolved Hide resolved
@xJonathanLEI
Copy link
Owner

@tcoratger Yes the codegen file should not be modified in any case, as any codegen update will overwrite all the changes.

@aniketpr01 Please make changes to xJonathanLEI/starknet-jsonrpc-codegen such that the codegen output matches exactly what this PR does to that file.

@aniketpr01
Copy link
Contributor Author

aniketpr01 commented Jun 20, 2024

@tcoratger Yes the codegen file should not be modified in any case, as any codegen update will overwrite all the changes.

@aniketpr01 Please make changes to xJonathanLEI/starknet-jsonrpc-codegen such that the codegen output matches exactly what this PR does to that file.

@xJonathanLEI should i keep the codegen changes as it is in this repo and perform same changes to starknet-jsonrpc-codegen by creating a seperate PR to it?
How do i ensure that codegen output matches exactly? by just changing codegen file or adding all the linters that are added for starknet-rs as well to that?

@xJonathanLEI
Copy link
Owner

should i keep the codegen changes as it is in this repo and perform same changes to starknet-jsonrpc-codegen by creating a seperate PR to it?

Yes please.

How do i ensure that codegen output matches exactly? by just changing codegen file or adding all the linters that are added for starknet-rs as well to that?

You can run the generate command and overwrite the file to this PR branch to see if you generate any diff. You should only be seeing a single line of commit hash diff.

An alternative solution would be to exclude the codegen file from the lints. Not sure if that's desirable though, as some lints actually affect the API (e.g. const).

@aniketpr01
Copy link
Contributor Author

should i keep the codegen changes as it is in this repo and perform same changes to starknet-jsonrpc-codegen by creating a seperate PR to it?

Yes please.

How do i ensure that codegen output matches exactly? by just changing codegen file or adding all the linters that are added for starknet-rs as well to that?

You can run the generate command and overwrite the file to this PR branch to see if you generate any diff. You should only be seeing a single line of commit hash diff.

An alternative solution would be to exclude the codegen file from the lints. Not sure if that's desirable though, as some lints actually affect the API (e.g. const).

@xJonathanLEI
Could you please respond the Telegram DM i have sent for a doubt over working with starknet-jsonrpc-codegen repo

@aniketpr01
Copy link
Contributor Author

aniketpr01 commented Jun 27, 2024

should i keep the codegen changes as it is in this repo and perform same changes to starknet-jsonrpc-codegen by creating a seperate PR to it?

Yes please.

How do i ensure that codegen output matches exactly? by just changing codegen file or adding all the linters that are added for starknet-rs as well to that?

You can run the generate command and overwrite the file to this PR branch to see if you generate any diff. You should only be seeing a single line of commit hash diff.

An alternative solution would be to exclude the codegen file from the lints. Not sure if that's desirable though, as some lints actually affect the API (e.g. const).

Have reverted codegen.rs from this(starknet-rs) repo and added another PR which will exclude existing rules to it.
PR: xJonathanLEI/starknet-jsonrpc-codegen#5
Please review @xJonathanLEI

@xJonathanLEI
Copy link
Owner

Squashed and rebased.

@xJonathanLEI xJonathanLEI merged commit a8ee4e2 into xJonathanLEI:master Jul 16, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more clippy lint rules
3 participants